Fix/ensure change output #152
Conversation
Tibo-lg
force-pushed
the
fix/ensure-change-output
branch
3 times, most recently
from
7b3d4eb to
9f6bc31
Compare
dlc-manager/src/utils.rs
Outdated
| // We need to have a change output, if we didn't on first try, we request an amount which | ||
| // includes minimum value for the change output as well as the fee for it. | ||
| if total_value < appr_required_amount + min_change_value + inputs_fee + change_fee { |
There was a problem hiding this comment.
Why do you think the implementer of get_utxos_for_amount should ever run into this? Isn't that just a bug in their implementation of the trait method?
I see the motivation, but I think this is a bit surprising. We can work on making the API easier to use and understand, but I think we shouldn't have to fix their mistakes by trying again with a slightly bigger amount here.
There was a problem hiding this comment.
So your proposal is just to error if there is not enough for change?
There was a problem hiding this comment.
I think that's okay, yeah. I know we have dealt with stuff like this, but I can't really blame rust-dlc if the API is clear and working as intended.
There was a problem hiding this comment.
Updated to return an error.
There was a problem hiding this comment.
To be honest I'm still pondering on this. Wouldn't it be easier to just request an amount that includes a change output + minimum spendable amount for it from the beginning?
There was a problem hiding this comment.
I think that's reasonable too and it seems simpler than what we are currently doing. I remember thinking that it might be a good idea to give more control to consumers of this API, but I can't figure out why now.
Tibo-lg
force-pushed
the
fix/ensure-change-output
branch
2 times, most recently
from
3c46ce4 to
29233b4
Compare
|
Cool @Tibo-lg Can you remind me what problem this was fixing again? It's been a while since I looked at this code, but I do remember we had some trouble with it |
Tibo-lg
force-pushed
the
fix/ensure-change-output
branch
from
29233b4 to
97af684
Compare
Tibo-lg
force-pushed
the
fix/ensure-change-output
branch
from
97af684 to
fe3346b
Compare
@sosaucily I finally went with a version that handles the issue within the library. If you have time to take a look let me know.